Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

server: add Uptime status counter #8888

Closed
wants to merge 3 commits into from
Closed

server: add Uptime status counter #8888

wants to merge 3 commits into from

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Dec 30, 2018 •
edited
Loading

What problem does this PR solve?

Fixes #8842

What is changed and how it works?

Uses variable infrastructure to push Uptime statistic. So you can now do this, as in MySQL:

tidb> show global status like 'Uptime';
+---------------+-------+
| Variable_name | Value |
+---------------+-------+
| Uptime        | 10    |
+---------------+-------+
1 row in set (0.00 sec)

Check List

Tests

  • Manual test

I would like to add a test, but I wanted to check first - since the logical way to do it is to run show global status, sleep a second, and then run it again. But I don't know if:

  1. Sleeping for 1 second is considered acceptable since it slows down the tests.
  2. It handles the race condition successfully that sleeping is never exactly 1 second.

As an alternative, I could assert that the value is greater than zero? Feedback welcome.

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation (supported statuses, variables)
  • Need to be included in the release note

This change is Reviewable

@morgo morgo mentioned this pull request Dec 30, 2018
Closed
@codecov-io
Copy link

codecov-io commented Jan 15, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #8888 into master will increase coverage by <.01%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8888      +/-   ##
==========================================
+ Coverage   67.25%   67.25%   +<.01%     
==========================================
  Files         372      372              
  Lines       76301    76203      -98     
==========================================
- Hits        51316    51253      -63     
+ Misses      20453    20418      -35     
  Partials     4532     4532
Impacted Files Coverage Δ
server/stat.go 0% <0%> (ø) ⬆️
server/server.go 53.15% <100%> (+0.02%) ⬆️
ddl/delete_range.go 74.28% <0%> (-4.58%) ⬇️
executor/join.go 78.44% <0%> (-1.04%) ⬇️
executor/distsql.go 72.53% <0%> (-0.47%) ⬇️
util/systimemon/systime_mon.go 100% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41838ce...4b47b61. Read the comment docs.

@wuudjac
Copy link
Contributor

wuudjac commented Feb 27, 2019

@morgo How about testing like this:

server, err := NewServer(...)
// Initialization stuff...

server.startTime = server.startTime.Add(-10 * time.Second)

result, err := server.Stats(...)
// Assert `result` got is not less than 10.

Since you are calculating duration, so you can change the lower bound instead of the upper bound.

For case 1, it will not slow down the tests.
For case 2, you can assert result is not less than any value you set, but cannot assert the upper limit in race condition.

If you want to assert upper limit, or want exact equal assertion, maybe you need to implement a mock time producer that offers time.Now()

zz-jason
zz-jason reviewed Jun 5, 2019
View reviewed changes
server/stat.go
@@ -0,0 +1,37 @@
// Copyright 2018 PingCAP, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Copyright 2018 PingCAP, Inc.
// Copyright 2019 PingCAP, Inc.

@zz-jason
Copy link
Member

zz-jason commented Aug 3, 2019

@morgo friendly ping, any update?

@sre-bot
Copy link
Contributor

sre-bot commented Aug 3, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@zz-jason zz-jason added contribution This PR is from a community contributor. status/ReqChange labels Aug 3, 2019
@zz-jason
Copy link
Member

@jackysp Since we have monitored the uptime on Grafana, maybe this PR is not that necessary?

@jackysp
Copy link
Member

jackysp commented Aug 19, 2019

This PR still has some compatibility improvements. But not so important. Maybe we can close it first? @zz-jason

@zz-jason
Copy link
Member

I'm going to close this PR due to outdated, fell free to reopen it if you are going to process it again in the future.

@zz-jason zz-jason closed this Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

Assignees
No one assigned
Labels
component/server contribution This PR is from a community contributor. type/compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support global status like 'uptime'
6 participants
@morgo @codecov-io @wuudjac @zz-jason @sre-bot @jackysp